Skip to content

πŸ› fix(contribute): make the task lease outlive the hub process (#5681) - #5688

Merged
kubestellar-prow[bot] merged 2 commits into
hivecommons:v5from
Danathar:fix/5681-lease-survives-restart
Sep 2, 2026
Merged

πŸ› fix(contribute): make the task lease outlive the hub process (#5681)#5688
kubestellar-prow[bot] merged 2 commits into
hivecommons:v5from
Danathar:fix/5681-lease-survives-restart

Conversation

@Danathar

@Danathar Danathar commented Sep 2, 2026

Copy link
Copy Markdown
Contributor

Summary

A hub restart threw away every in-flight contributor task, then handed the identical issue back seconds later.

A contributor relay holds one task at a time and keeps working through a brief disconnect, re-asserting the task when it reconnects. The hub only honours that re-assertion if it can match a server-issued lease β€” deliberate, since #C4: a client must never be able to assert ownership of work the server did not assign. But leases lived only in the hub's memory. A restart emptied the registry, so after an upgrade no in-flight resume could match, the relay was told no active lease for this task, and the revoke interrupted a working agent mid-turn.

The waste was visible in the ordering: revoked at 14:24:40, the same issue re-assigned to the same relay at 14:24:44. Ownership was never in question β€” only the record of it. Self-upgrade rolls (#5391) make this routine rather than rare, and it hits every contributor holding a task at the moment of any restart.

The fix is option (1) from the issue: persist the lease. It is written to /data/contributors/task-leases.json on every assignment, renewal and release, and read back at startup, so a restart becomes just a longer-than-usual disconnect.

Why this does not weaken C4

The security posture was "never rebuild ownership from client-supplied fields". That never required the record to be volatile β€” only to be the server's:

  • The restored record is one the hub itself wrote, and a resume is matched against it exactly as before, on {identity, task_id, repo, number, generation} and inside the window. The test asserts all six mismatch cases (wrong task / generation / repo / number / identity, and an unversioned gen == 0) are still refused after a restart.
  • No credential is in the file. The scoped GitHub token is minted per assignment and delivered separately (contribute: gate contributor credentials on explicit task acceptanceΒ #2537). Restoring a lease grants the ability to re-adopt a task the hub already issued, never to obtain a credential without passing selectTask.
  • Written 0600, unlike the sibling contributor ledgers' 0644 β€” this is an authorization record, not a report.
  • A lease already past its expiry is dropped at load, not restored, so a stale file cannot resurrect a task that is no longer re-adoptable. A malformed file degrades to "no leases" β€” the pre-fix behavior.

Three couplings that had to move with it

Persisting the map alone would have been quietly wrong in three ways:

  1. The renewed window is what gets persisted. Persisting only the assignment-time window would bring a task that had been progressing for longer than leaseTTL back already expired β€” precisely the defect A relay that reconnects inside the backoff window cannot resume its task β€” in-flight work is interrupted mid-turnΒ #4260 fixed in memory.
  2. taskGen is advanced past every restored generation at boot. It is an in-memory counter that restarts at zero, so persisting leases without this would let a post-restart assignment mint a fencing token that aliases a restored one, and the contributor: task ownership has no hub lease or safe operator revoke/requeue pathΒ #2568 Gate would accept a pre-restart straggler against a brand-new task. src/docs/design/agent-state-inventory.md residual 2 called this hazard out in advance β€” "any change that persists one without the other (e.g. 'let's make leases survive restarts') silently breaks the fence" β€” so that residual is now marked resolved, with the coupling stated in loadLeases and pinned by a test rather than carried only by that document.
  3. A restored lease holds its work item for two minutes after boot. The double-assignment guard is built purely from live connections, which is empty right after a restart. Making the resume work without this would convert the old "lose the task" bug into two relays on one issue β€” which is the other half of the contract the issue asks to pin.

That third point is deliberately scoped, and the scoping is the part most worth reviewing. A lease is not a hold in steady state: a dropped socket keeps its lease so the relay can resume (#4260) while its item merely cools down (#2356's speculative hedge). Honouring leases as holds for the full 30-minute TTL would silently replace that hedge with a long park on every disconnect β€” a policy change nobody asked for. So the hold applies only to leases restored from the previous process, only for leaseHoldGraceAfterStart (2 min, against a 1-second relay reconnect backoff), and never to the requester's own lease. Leases minted by this process are never holds; their holders have live connections the existing guard already covers.

The lease also now carries the item's canonical worksource key, so the guard recognises external work β€” Linear/Jira items carry Number == 0 and put their identity in Key (#4245) β€” instead of colliding every such item as repo#0 (#5120).

Cluster claimed (files/functions)

  • src/pkg/dashboard/contribute_ws.go β€” taskLease gains key/restored; recordLeaseForKey; save hooks in recordLease/renewLease/revokeLease/lookupLease's expiry drop; new persistedLease, saveLeasesLocked, loadLeases, pruneExpiredLeases, leasedIssueKeys; taskLeasesFile/startedAt on the hub; loadLeases at construction; prune in cleanupLoop; the lease exclusion in selectTask.
  • src/pkg/dashboard/contribute_lease_restart_test.go (new, 11 tests), api_contribute_test.go (redirect the new file in redirectContributeWSDisk).
  • src/docs/contributor-relay.md, src/docs/design/agent-state-inventory.md (rows 31/32 + residual 2), CHANGELOG.md.

Disjoint from the open hold-gated PRs β€” none of them touch pkg/dashboard/contribute_ws.go. Also disjoint from my own #5682 (pkg/escalation, pkg/scheduler, cmd/hive).

Validation

  • go build ./..., go vet ./pkg/dashboard/, gofmt -l clean on every touched file. (gofmt flags eight other pkg/dashboard test files β€” pre-existing drift, not touched here.)
  • go test ./pkg/dashboard/ passes in full, including TestSelectTask_DeclaredCapabilitiesDoNotAffectSelection, which is the test that catches an over-broad lease exclusion: a blanket hold fails it, the restart-scoped one does not.
  • src/scripts/check-docs-links.py: 130 files, all links and anchors resolve.
  • Mutation-checked, three ways β€” each mechanism is independently pinned, and none of the new tests passes vacuously:
    • Remove loadLeases() -> 5 tests fail, the end-to-end one on the incident's own string: the relay reconnecting after a hub restart was told "no active lease for this task".
    • Drop the persist-on-renew -> only TestLeaseRestart_RenewedWindowSurvives fails.
    • Drop the restored-lease hold -> only TestLeaseRestart_RestoredLeaseHoldsItsIssue fails.

Not claimed

Option (3) from the issue β€” drain in-flight tasks before exiting β€” is not here. The issue calls it complementary rather than a substitute, and it is: it shrinks the window but cannot close it, since an upgrade will not wait out a 20-minute turn. It is also a change to shutdown sequencing rather than to the lease, and belongs in its own PR. Option (2) (reconstruct from the task-run log) is not needed once (1) lands.

Related issues

Refs #5681. Related: #4260 (the resume contract this extends across a process boundary), #5390 (the clean close that precedes it), #5391, #5120, #4245, #2568, #2537, #2356.

Testing

  • cd src && go build ./...
  • cd src && go test ./... β€” ran ./pkg/dashboard/ in full (the only package touched); passes.

β€” hive: backend=claude model=claude-opus-5

@kubestellar-prow kubestellar-prow Bot added dco-signoff: yes Indicates the PR's author has signed the DCO. needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. size/XL Denotes a PR that changes 500-999 lines, ignoring generated files. labels Sep 2, 2026
A hub restart threw away every in-flight contributor task β€” and then handed the
identical issue back seconds later.

A relay holds one task at a time and keeps working through a brief disconnect,
re-asserting the task when it reconnects. The hub honours that re-assertion only
against a server-issued lease (lookupLease), which is deliberate: C4 established
that a client must never be able to assert ownership of work the server did not
assign. But leases lived only in process memory. A restart emptied the registry,
so after an upgrade NO in-flight resume could match: the relay was told "no
active lease for this task", the revoke interrupted the agent mid-turn, and the
same issue was re-assigned as fresh work.

Observed 2026-09-02 (hivecommons#5681): revoked at 14:24:40, the same issue reassigned to
the same relay at 14:24:44, discarding two and a half minutes of a turn that was
progressing normally. Thirteen shell commands and a fork/clone in, then killed.
Ownership was never in question β€” only the record of it. Self-upgrade rolls
(hivecommons#5391) make this routine rather than rare, and it hits every contributor
holding a task at the moment of any restart.

The registry is now persisted to /data/contributors/task-leases.json on every
assignment, renewal and release, and restored at startup.

This does not weaken C4. The restored record is one the SERVER wrote; a resume
still has to match it exactly on {identity, task_id, repo, number, generation}
and still has to be inside the window. Nothing is reconstructed from
client-supplied fields, the file carries no credential (the scoped token is
minted per assignment and delivered separately, hivecommons#2537), it is written 0600
rather than the sibling ledgers' 0644 because it is an authorization record
rather than a report, and a lease already past its expiry is dropped at load
rather than restored.

Three couplings move with it:

- The RENEWED window is what gets persisted. Persisting only the assignment-time
  window would bring a task that had been progressing for longer than leaseTTL
  back already expired β€” precisely the defect hivecommons#4260 fixed in memory.
- taskGen is advanced past every restored generation at boot. It is an in-memory
  counter that restarts at zero, so persisting leases without this would let a
  post-restart assignment mint a fencing token that ALIASES a restored one, and
  the hivecommons#2568 Gate would accept a pre-restart straggler against a brand-new task.
  src/docs/design/agent-state-inventory.md residual 2 called this exact hazard
  out in advance ("any change that persists one without the other silently
  breaks the fence").
- For leaseHoldGraceAfterStart (2 min) after boot, a RESTORED lease also holds
  its work item in the double-assignment guard, which is otherwise built purely
  from live connections and is empty right after a restart. Without it, making
  the resume work would convert "lose the task" into two relays on one issue.
  Deliberately scoped: a lease is not a hold in steady state β€” a dropped socket
  keeps its lease so the relay can resume (hivecommons#4260) while its item merely cools
  down (hivecommons#2356), and honouring leases as holds for the full 30-minute TTL would
  silently replace that hedge with a long park on every disconnect. Leases
  minted by this process never act as holds; their holders have live
  connections, which the existing guard already covers.

The lease also now carries the item's canonical worksource key, so the guard
recognises external work (Linear/Jira items carry Number == 0 and put their
identity in Key, hivecommons#4245) instead of colliding every such item as "repo#0" (hivecommons#5120).

hivecommons#4260's contribute_reconnect_resume_test.go pins the resume contract across a
SOCKET drop and passed throughout β€” that reconnect is to a live hub. The new
contribute_lease_restart_test.go exercises the same contract across a PROCESS
boundary, which had no coverage: it drives the real protocol through a hub
replaced by a freshly constructed one over the same /data, and fails on the
incident's own string ("no active lease for this task") when the restore is
removed.

Refs hivecommons#5681

Signed-off-by: Doug Baggett <doug.baggett@gmail.com>
- contributor-relay.md: the resume section already documented the disconnect
  case and the exact four-line revoke/reassign symptom. Adds the restart case
  beside it, including that the restored record is still hub-written and still
  matched exactly, and the two-minute post-restart hold.
- agent-state-inventory.md: rows 31 (task leases) and 32 (taskGen) move from
  volatile to fixed. Residual 2 warned that persisting one without the other
  would silently break the generation fence β€” that warning was load-bearing and
  is now marked resolved, with the coupling stated in loadLeases and pinned by
  TestLeaseRestart_GenerationAdvancesPastRestoredLeases rather than carried only
  by that document.
- CHANGELOG.md: user-visible β€” in-flight contributor work now survives an
  upgrade roll.

Refs hivecommons#5681

Signed-off-by: Doug Baggett <doug.baggett@gmail.com>
@Danathar
Danathar force-pushed the fix/5681-lease-survives-restart branch from f30986a to bb3812a Compare September 2, 2026 15:30
@kubestellar-prow kubestellar-prow Bot removed the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Sep 2, 2026
@clubanderson

Copy link
Copy Markdown
Member

Review β€” OK to merge on green. Adjudicated against #5693 (same #5681 fix, v4 base); closed that one in favor of this β€” it missed the taskGen re-derivation the agent-state-inventory doc warns is load-bearing, the post-restart double-assignment window, write-under-lock ordering, and 0600.

Verified here:

  • Restart round-trip is driven through the real protocol (assign β†’ new hub over the same /data β†’ reconnect + re-assert), and fails on the actual incident frames (revoke, re-offer) rather than a proxy.
  • No resurrection after legitimate expiry: expired records are skipped at save, dropped at load, and reaped by pruneExpiredLeases in cleanupLoop; revokes persist immediately so a released task cannot come back.
  • TTL clock: the renewed window is what persists (the A relay that reconnects inside the backoff window cannot resume its task β€” in-flight work is interrupted mid-turnΒ #4260 case across a process boundary), so a long-running task does not come back pre-expired.
  • Fence: taskGen is advanced past every restored generation at boot with a pinning test β€” closing residual 2 in the inventory doc rather than tripping it.
  • Grace hold: restored leases hold their items for 2 minutes only, never for the requester's own lease, and a lease minted in-process is never a hold β€” so steady-state lease semantics (A relay that reconnects inside the backoff window cannot resume its task β€” in-flight work is interrupted mid-turnΒ #4260//contribute sending "dupe" PRsΒ #2356) are unchanged.
  • Locking: saveLeasesLocked is only ever called under leaseMu and takes nothing else; no re-entrancy, and the write-under-lock choice correctly prevents rename reordering.
  • C4 exact-match is untouched (negative cases tested), file is 0600, malformed file degrades to empty.

Two non-blocking follow-ups: (1) the persist idiom is fixed-name path+".tmp" + rename with no fsync β€” short of the #5625 standard (unique CreateTemp + chmod + fsync file and dir). Tolerable here because a lost last write degrades to the pre-fix behavior, but this ledger (and its siblings) should be aligned with #5625 in a follow-up. (2) v4 carries the same registry and fence and needs a port of this change β€” including the taskGen re-derivation.

@clubanderson clubanderson added lgtm Indicates that a PR is ready to be merged. approved Indicates a PR has been approved by an approver from all required OWNERS files. labels Sep 2, 2026
@kubestellar-prow

Copy link
Copy Markdown
Contributor

[APPROVALNOTIFIER] This PR is APPROVED

Approval requirements bypassed by manually added approval.

This pull-request has been approved by:

The full list of commands accepted by this bot can be found here.

The pull request process is described here

Details Needs approval from an approver in each of these files:

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

@Danathar

Danathar commented Sep 2, 2026

Copy link
Copy Markdown
Contributor Author

Note for whoever tracks backports: this merged to v5, but #5681 was reported against a v4-deployed hub and loadLeases is absent from origin/v4.

The failure recurred today after this merged. The hosted spoke serving a2b71cb (which is on origin/v4) restarted for an upgrade at 16:38:34Z and took an in-flight contributor task with it:

[12:38:34] Connection closed (code=1012 service restart: hub restarting for upgrade)
[12:38:35] Reconnected while working on kubestellar/hive#5701 β€” resuming
[12:38:35] Task revoked: ct-kubestellar/hive-5701-1788366752 β€” no active lease for this task
[12:38:40] Task assigned: issue kubestellar/hive#5701   <- same issue, new id, gen=2

The agent had already printed its HIVE_VERDICT for that task; the verdict was discarded and the work redone from scratch, costing ~6 minutes. Second occurrence today (the first cost the #5617 turn at 16:24Z).

Flagging only so the v4 gap is visible β€” the fix itself is what the issue asked for.

kubestellar-prow Bot added a commit that referenced this pull request Sep 2, 2026
…istence

πŸ› fix(contribute): persist task leases across hub restarts (v4 port of #5688)
clubanderson pushed a commit that referenced this pull request Sep 3, 2026
A contributor relay works one issue at a time out of a single persistent
checkout, and nothing resets that checkout between tasks. The task prompt
told the agent to fork, clone, commit, push and open a PR, but mentioned a
branch exactly once β€” "push your branch to your fork remote" β€” and never
said which branch to start from or target. The base was therefore whatever
the previous task happened to leave checked out.

With two active branches that is enough for one branch-specific issue to
redirect every later PR of a session. On 2026-09-02 issue #5617, titled
"[v5] reviewer lane follow-ups …", correctly put the checkout on v5; the
four PRs after it (#5688, #5700, #5705, #5711) inherited v5, and three of
them were fixes for defects live on the deployed v4. Branch ancestry
confirms inheritance rather than choice β€” each is 1–2 commits ahead of v5
and 64–67 ahead of v4, with no base_ref_changed event on any of them. The
cost was real: #5688 and the scanner's independent #5693 fixed the same
defect on two branches, a maintainer had to adjudicate between them, and
the v4 fixes were backported by hand hours later.

The failure is invisible from every seat. The agent has nothing to check
against, the contributor sees PRs opening and merging normally, and a
maintainer sees correctly-formed PRs on a plausible branch.

buildTaskPromptBody now names the base and tells the agent to start its
work branch from it ('git checkout -b <branch> upstream/<base>'), open the
PR with 'gh pr create --base <base>', and confirm the PR's base before
reporting done. The base comes from taskBaseBranch: the branch this hive
was built from β€” the same upstreamBranch() the onboarding page's clone
command already names (#3990), so the two answers agree β€” unless the issue
title carries a release-line tag such as "[v5]", which wins. The tag shape
is narrow on purpose ('v' plus digits, the shape image_pulls.go matches and
.github/release-lines.yml lists), so the lane prefixes the classifier
routes on ("[quality]", "[architect]") cannot be read as branches.

The prompt is the load-bearing half. Fixing only the workspace was measured
and found insufficient: a working branch reset from v5 onto v4 mid-task,
holding zero commits and a clean tree, was restored to v5 by the agent,
because the plan it had already formed said v5. An agent follows what it
was told over what it finds, so the instruction has to carry the answer.

When no base resolves at all, the prompt still refuses inheritance and
names the substitute β€” the upstream repository's own default branch β€”
rather than falling silent.

Not included, and deliberately: resetting the workspace to the base before
each task. bin/contributor-relay.sh performs no git operations today (it
sets a cwd and types a prompt), so that defence in depth means a new
protocol field plus a 'git reset --hard' against a contributor's persistent
checkout β€” a destructive new surface that deserves its own change and its
own observation, not a rider on this one.

Regression coverage in contribute_task_base_branch_test.go asserts the
prompt names a base, that a task following a branch-specific one is told
this hive's branch rather than the previous task's, that the base is
derived rather than re-hardcoded, that an uninjected build branch falls
back instead of emitting "unknown", that an unresolvable base still forbids
inheritance, and that lane prefixes are not mistaken for release lines.

Fixes #5729

Signed-off-by: Danathar <doug.baggett@gmail.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

approved Indicates a PR has been approved by an approver from all required OWNERS files. dco-signoff: yes Indicates the PR's author has signed the DCO. lgtm Indicates that a PR is ready to be merged. size/XL Denotes a PR that changes 500-999 lines, ignoring generated files.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants